Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Nvt pref name #394

Merged
merged 11 commits into from
Feb 20, 2019
Merged

Nvt pref name #394

merged 11 commits into from
Feb 20, 2019

Conversation

kroosec
Copy link
Contributor

@kroosec kroosec commented Feb 12, 2019

Change NVT preferences format to OID:PrefType:PrefName. This allows plugins to change their script names without breaking existing configs.

PR contains changes needed to be able to rebuild/update nvt's and run scans with greenbone/openvas-scanner#275 using default configs (+ docmentation updates, changes I did on my way while grepping the code for hardcoded values etc,.)

Still needed:

  • Migrators for existing scan configs.
  • Code for pattern matching that uses sscanf() calls could be simplified and made more readable by using g_str_split(pref, ":", 2) instead, but I didn't want to make the PR more intrusive.

@kroosec kroosec mentioned this pull request Feb 12, 2019
Merged
timopollmeier
timopollmeier reviewed Feb 14, 2019
View reviewed changes
Copy link
Member

@timopollmeier timopollmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think with hardcoded OIDs it's harder to tell what NVTs are, for example to check for typos and
copy-paste errors, so I think it may be a good idea to define macros for them.

Also, are you going to add the migrator yourself or would you prefer me or Matt to implement it?

@kroosec kroosec mentioned this pull request Feb 15, 2019
Merged
2 tasks
@kroosec kroosec force-pushed the nvt-pref-name branch 2 times, most recently from ed58707 to e544dc8 Compare February 15, 2019 14:43
@kroosec
Copy link
Contributor Author

kroosec commented Feb 15, 2019

I think with hardcoded OIDs it's harder to tell what NVTs are, for example to check for typos and
copy-paste errors, so I think it may be a good idea to define macros for them.

Also, are you going to add the migrator yourself or would you prefer me or Matt to implement it?

Thanks for the review @timopollmeier. I updated the PR to use #define's for the hardcoded values. I prefer if you do the migrator, as I may miss something there.

timopollmeier
timopollmeier requested changes Feb 15, 2019
View reviewed changes
Copy link
Member

@timopollmeier timopollmeier left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also noticed now that the init_nvt_preference_iterator() function would also need to be changed to take the OID as a parameter and to use the new format.
The other NVT preference iterator functions getting parts of the name should also be updated.

@kroosec
Copy link
Contributor Author

kroosec commented Feb 15, 2019

I also noticed now that the init_nvt_preference_iterator() function would also need to be changed to take the OID as a parameter and to use the new format.
The other NVT preference iterator functions getting parts of the name should also be updated.

Fixed in c7ef673

@timopollmeier
Migrate database to version 206
c1a78ea 
This will convert the names of NVT preferences to the new format using
the OID instead of the name to identify the NVTs.
timopollmeier and others added 10 commits February 20, 2019 10:07
@kroosec kroosec merged commit b6d078e into greenbone:master Feb 20, 2019
@mattmundell mattmundell mentioned this pull request Sep 8, 2019
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timopollmeier timopollmeier timopollmeier approved these changes

@mattmundell mattmundell Awaiting requested review from mattmundell

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kroosec @timopollmeier